-
Notifications
You must be signed in to change notification settings - Fork 4
Update default discovery and refresh config values #191
Conversation
Is this because mira is chatty? I'm just thinking around when in production if you'd rather not have the shorter values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment on the selected interval
const defaultEngineDiscoveryRefreshRate = 1000; | ||
const defaultEngineHealthRefreshRate = 5000; | ||
const defaultEngineDiscoveryRefreshRate = 10000; | ||
const defaultEngineHealthRefreshRate = 30000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not a bit too infrequent? I cannot tell what is right but every 30 s introduces quite some delay until an unhealthy engine is detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docker default healthcheck interval for Mira is 30 seconds so that is why I chose that value.
This value in Config.js
is also used as the interval for Mira to fetch metrics from each engine. Atleast in my opinion 5 seconds is a bit greedy for both health and metrics, and if a shorter interval is needed it is still configurable using env variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, reasonable intervals.
We may need additional logic to update individual engines at some point based on external information (like rabbitmq, APIs) which will give us lower refresh rates automagically anyway.
Mira has a very short interval for discovering and checking health on engines. Changed the interval to: